Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deli read client fixes #9925

Merged
merged 3 commits into from
Apr 25, 2022
Merged

Deli read client fixes #9925

merged 3 commits into from
Apr 25, 2022

Conversation

GaryWilber
Copy link
Contributor

@GaryWilber GaryWilber commented Apr 16, 2022 •
edited
Loading

Follow up on #9191. Ran into a few issues once I started extensive testing in Push.

The ExtendClient control message had a few issues:

  1. It only allows 1 client to be extended at a time. So if alfred had 100 read clients connected, it would have to send 100 control messages to deli every ~5mins. That's not ideal.
  2. The expiration was done using a lastUpdateTime property, which was set to Date.now() when the ExtendClient message shows up. While this does work, it's kind of annoying. If delis clientTimeout was updated, it was possible for it to immediately cause all read clients to be kicked out. I would rather the clientTimeout time to only take effect the next time the client was extended.
  3. It extended the clients expiration time by updating the property on the object in the readClients map. However it didn't update the expiration time for the client that's in the database (Redis), which is managed by ClientManager. That means a kafka rebalance, which recreates the deli lambda, would fail to see that the client was extended since it's reading the clients from redis on startup.

Fixes for each of the above issues:

  1. The control message can now take an array of client ids.
  2. Replaced lastUpdateTime with exp time. This also aligns more with how Redis expiration times work.
  3. ClientManager is now passed to deli and it contains an extendClients method. Deli will call that when the control message shows up. The method should extend the TTL of the clients in Redis and update the exp property for the deli client objects. Deli will also now keep track of the sequence numbers within the ReadClient object. That makes the Redis commands to extend the client simpler (no longer needs to fetch the existing client object).

Other fixes / changes:

  • Fixed deli not removing readClients from it's in-memory map.
  • Rename ITimedClient to ISequencedSignalClient to better align with the naming scheme of things in ClientManager.

Extra notes for reference:

  • Read clients require the alfred to send ExtendClient messages to deli
  • Write clients don't require that. We don't have to worry about write client expiration because write clients inherently "expire" when deli removes the client from the quorum.
  • The easiest way to store these clients in Redis is to just store the ISequencedSignalClient object as is.

@GaryWilber GaryWilber requested a review from hedasilv April 16, 2022 00:01
@GaryWilber GaryWilber requested a review from a team as a code owner April 16, 2022 00:01
@github-actions github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Apr 16, 2022
hedasilv
hedasilv reviewed Apr 18, 2022
View reviewed changes
server/routerlicious/packages/services-core/src/clientManager.ts Outdated
@@ -40,5 +59,15 @@ export interface IClientManager {
* Returns all clients currently connected including a keep alive time.
* Should be used with delis read only client functionality.
*/
getTimedClients?(tenantId: string, documentId: string): Promise<Map<string, ITimedClient>>;
getSequencedClients?(tenantId: string, documentId: string): Promise<Map<string, ISequencedSignalClient>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSequencedClients and extendSequencedClients are both optional, so that's why I think we did not have to immediately update the Redis ClientManager (and other test implementations) to include these. If r11s or FRS want to enable this new set of features being added for read clients, I assume we would have to provide an implementation, correct?

Also, let's say we mistakenly enabled this new set of read client features (I think we had a setting for that in the other PR). Since getSequencedClients and extendSequencedClients are both optional, our build would not break if we do not provide an implementation. But how would the service behave in that case?

In other words, I'm just concerned that if we check this in without providing concrete implementations to getSequencedClients and extendSequencedClients, we might later forget about them when we enable the read client feature, and we won't have a build error blocking us to make us realize that issue.

Would you say it is a good idea to make extendSequencedClients and possibly getSequencedClients be mandatory, and include implementations of that as part of this PR? I can also help with that.

@pradeepvairamani @znewton can you also take a look at this and let me know if I'm missing something from our side? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you would need to implement these in order to use this new read client stuff.

I could make the methods required and cause them to throw "Not Implemented" errors for now. That way you would be required to implement them when moving to this system since you would get errors when trying to use it. Would that work?
Since I don't use any of the RedisClientManager / test client manager code, I don't think I should be the one to implement those methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes total sense. We will follow up later to provide the actual implementations in Redis ClientManager. For now, having the "Not Implemented" error thrown is already helpful as we will definitely see it happening if we enable the Deli read client feature.

@GaryWilber GaryWilber mentioned this pull request Apr 19, 2022
Closed
hedasilv
hedasilv approved these changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@hedasilv hedasilv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @GaryWilber ! I ran r11s in Docker using your repo fork/branch and can confirm all works as expected (while the feature is disabled). And later, when we enable it, we should see the error messages in the Redis ClientManager, reminding us to implement the required methods.

@GaryWilber GaryWilber merged commit ac0add6 into microsoft:main Apr 25, 2022
@GaryWilber GaryWilber deleted the deli_read_client_exp_time branch April 25, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hedasilv hedasilv hedasilv approved these changes

Assignees
No one assigned
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GaryWilber @hedasilv